Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: add markdown linting and spellcheck #186

Merged
merged 20 commits into from
Jul 14, 2023

Conversation

danceratopz
Copy link
Member

@danceratopz danceratopz commented Jul 4, 2023

This PR:

  1. Adds markdown spellchecking and linting to tox and Github Actions.
  2. Fixes existing linting problems and spelling in docs/**/*.md & README.md.

Motivation

We now have CD of our docs from the main branch and we should try and maintain a certainly level of quality:
https://ethereum.github.io/execution-spec-tests/main

External Tooling Required

  • spelling: Uses the pyspelling Python cli tool which requires the external aspell command. This is a standard package and can be installed on Debian-based distros via:
    sudo apt-get install aspell aspell-en
    
  • lint: Uses the markdownlint-cli2 command-line utility. This is node.js program, so introduces quite a hefty dependency. If you don't already use node.js (and require multiple versions), then you can install markdownlint with:
    sudo apt install nodejs
    sudo npm install markdownlint-cli2 --global
    
    Experienced users would probably use nvm to manage and install different node versions, see https://www.freecodecamp.org/news/node-version-manager-nvm-install-guide/.

Tooling and Integration with VS Code:

  • Markdown spellchecking is already covered in VS Code via the Code Spell Checker that we already recommend in our extensions.json.

  • Dictionaries: Now "whitelist.txt" is split into 3 separate wordlist files for maintainability. One reason: If you do "Quick Fix" -> "Add word to dictionary 'project-words'", VS Code sorts the wordlist alphabetically, which is not so optimal for our list of opcodes. Pyspelling supports multiple wordlists. flake8-spellcheck works only supports one wordlist file, which must be called whitelist.txt. This is generated using the src/entry_points/create_whitelist.py in tox prior to running flake8.

  • Btw, I did check whether we could spellcheck the markdown via the flake8-spellcheck plugin for simplicity, but it was not possible.

  • I used the vscode-markdownlint for a couple of weeks - it works well. This extension uses the markdownlint-cli2 and there's a corresponding Github Action that uses this tool.

If you use these two plugins and clean-up any errors, then tox should pass.

Soft Fail Mode Rational

It's not ideal to require external (non-native Python) tooling and these tools are only required for people who work heavily on the docs. As such, if these tools are not available, tox prints a hint that they're missing, but will still pass. Most devs will only be working on the tests and should not break the docs or need to change them (docstrings in tests will get spell-checked from flake8; linting the output is more difficult, perhaps there's a flake8 plugin extension?).

Keeping tox cross-platform (hopefully)

Additionally, the tools are executed via Python scripts in src/entry_points to avoid using external commands (e.g. bash) in tox to ensure it still runs cross-platform.

Github Actions

These checks will always be ran in Github actions:

  • Spellcheck is ran as part of tox (it's easy to install the aspell dependencies).
  • Lint is ran using DavidAnson/markdownlint-cli2-action@v11 to avoid dealing with node.js dependencies in the workflow.

@danceratopz danceratopz force-pushed the docs/add-linting-and-spellcheck branch from 830e7f7 to 76dd1a1 Compare July 4, 2023 11:19
@danceratopz danceratopz marked this pull request as ready for review July 6, 2023 06:58
@danceratopz
Copy link
Member Author

Would be good to get some feedback, and if you like it, I'll add some documentation before merge.

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, I think this will certainly help us maintain the quality of the docs :)

@danceratopz danceratopz force-pushed the docs/add-linting-and-spellcheck branch from a77f393 to c2fb90e Compare July 14, 2023 07:11
@danceratopz
Copy link
Member Author

Rebase due to:

Copy link
Collaborator

@spencer-tb spencer-tb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I really like the dictionaries - I feel like we are bulletproof now :D

@danceratopz danceratopz merged commit 6e31a12 into ethereum:main Jul 14, 2023
3 checks passed
@danceratopz danceratopz deleted the docs/add-linting-and-spellcheck branch September 28, 2023 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants